Ensure proxies respect command options by watersRand · Pull Request #3261 · redis/node-redis

watersRand · 2026-05-02T15:59:22Z

Description

Closes #3055 ,a bug where commands generated via static factories (Modules and Functions) were incorrectly referencing the root client's options (this._self._commandOptions) instead of the immediate instance's options (this._commandOptions).

The Problem:
When using .withCommandOptions() or .withTypeMapping(), the library creates a proxy object. However, because Modules and Functions were hard-coded to look at the internal _self reference for their configuration, they completely ignored any user-defined type mappings or timeouts set on the proxy. This effectively broke RESP3 type mapping for all Redis Module commands.

The Solution:

Updated #createModuleCommand and #createFunctionCommand to reference this._commandOptions.
Updated sendCommand to prioritize instance-level options.
Updated the NamespaceProxyClient type definition to include _commandOptions to ensure type safety.

Allows developers to seamlessly switch between different configurations—such as string vs. binary (Buffer) outputs—across all available commands, including built-in modules, third-party modules, and user-defined libraries (Redis Functions). It enables this flexibility on the fly without the overhead of re-initializing the entire connection or maintaining multiple client instances for different return types.

Checklist

Does npm test pass with this change (including linting)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Note

Medium Risk
Changes core option merging and dispatch for client, pool, and cluster; incorrect merging could affect timeouts, type mappings, and cluster redirection behavior, though coverage is broad.

Overview
Fixes command-option handling so withCommandOptions / withTypeMapping proxies actually affect dispatch for modules, functions, scripts, and raw sendCommand on standalone clients, pools, and cluster—not only the root _self client.

Layering: Proxies now stack options on a prototype chain (Object.assign(Object.create(...), overrides)) instead of shallow spreads. Merge at dispatch uses the same pattern so inherited keys like timeout and asap stay visible when later layers add typeMapping.

Routing: Namespace command factories and attachNamespace forward this._commandOptions (including a getter on module namespaces). duplicate() on client/cluster flattens the option chain into a plain object for new instances. Cluster _execute passes merged options through a per-execution client proxy so deep proxy chains still apply type mappings during routing.

Tests were updated/added for chained options, module echo with Buffer mapping, pool/cluster inheritance, and duplicate() preserving flattened options.

^{Reviewed by Cursor Bugbot for commit ffeefc8. Bugbot is set up for automated code reviews on this repo. Configure here.}

nkaradzhov · 2026-05-07T08:04:51Z

@watersRand thanks, I will have a look!

nkaradzhov

There are more places where this._self._commandOptions or this._self.commandOptions is used:

node-redis/packages/client/lib/client/index.ts

Line 293 in e4cc77e

    
           return this._self._executeCommand(command, parser, this._self._commandOptions, transformReply);

node-redis/packages/client/lib/client/index.ts

Line 306 in e4cc77e

    
           return this._self._executeCommand(fn, parser, this._self._commandOptions, transformReply);

node-redis/packages/client/lib/client/index.ts

Line 1186 in e4cc77e

...this._self._commandOptions,

node-redis/packages/client/lib/client/pool.ts

Line 145 in e4cc77e

    
           return this._self.execute(client => client._executeCommand(command, parser, this._self._commandOptions, transformReply))

node-redis/packages/client/lib/client/pool.ts

Line 158 in e4cc77e

    
           return this._self.execute(client => client._executeCommand(fn, parser, this._self._commandOptions, transformReply))    };

node-redis/packages/client/lib/cluster/index.ts

Line 177 in e4cc77e

this._self._commandOptions,

node-redis/packages/client/lib/cluster/index.ts

Line 195 in e4cc77e

this._self._commandOptions,

node-redis/packages/client/lib/cluster/index.ts

Line 525 in e4cc77e

...this._self._commandOptions,

node-redis/packages/client/lib/sentinel/utils.ts

Line 65 in e4cc77e

    
           client => client._executeCommand(fn, parser, this._self.commandOptions, transformReply)

node-redis/packages/client/lib/sentinel/utils.ts

Line 79 in e4cc77e

    
           client => client._executeCommand(command, parser, this._self.commandOptions, transformReply)

It would be great if we can fix and test those as well.

nkaradzhov · 2026-05-07T10:29:13Z

  });

+
+  testUtils.testWithClient('Module TypeMapping Fix', async client => {


maybe change the title to: "proxies respect command options"

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit fa944a8. Configure here.}

nkaradzhov · 2026-06-25T09:43:24Z

@watersRand thanks, i will have a look at this.

nkaradzhov

A few things are still open before this can go in:

RESP default regression (commander.ts). const RESP = config?.RESP ?? 2 should stay ?? DEFAULT_RESP (which is 3). The library default is RESP3, and this binds RESP2 transformReply handlers at class-generation time for any client created without an explicit RESP — even though the connection still negotiates RESP3. Bugbot re-flagged this on the latest commit. Please revert to DEFAULT_RESP (and keep the import).
Sentinel getter recurses / duplicate member (sentinel/index.ts). The added
get _commandOptions() { return this.commandOptions; }
recurses — the existing commandOptions getter already returns this._commandOptions, so commandOptions → _commandOptions → commandOptions → … overflows the stack. It also collides with the existing private _commandOptions? field (duplicate identifier), in both RedisSentinelClient and RedisSentinel. Sentinel already stores options in #commandOptions with a public commandOptions getter, so it needs a different shape than the standalone client. Suggest having the sentinel command funcs read this._self.commandOptions (the existing public getter) rather than inventing _commandOptions, or wiring the namespace _self the way commander.ts does — but not a self-referential getter.
Cluster withCommandOptions not updated (cluster/index.ts). Still proxy._commandOptions = options; (plain replace), unlike client/pool which now do Object.assign(Object.create(this._commandOptions ?? null), options). Since the cluster command paths now read this._commandOptions, chaining withCommandOptions drops parent options like timeout. Please make it consistent.
Duplicate import (cluster/index.spec.ts). import { RESP_TYPES } from '../RESP/decoder' appears twice. Won't compile.
Unrelated formatting churn. Most of the diff is reformatting (noop, if( → if (, reindenting the .on() chain and the _ejectSocket block, type-map indentation) in files this fix doesn't functionally touch. It buries the real change and invites conflicts — please strip it and keep only the functional edits. If formatting is genuinely off, a separate PR is the place for it.

Also minor: the title still says "ensure module commands respect proxy typeMapping" — "proxies respect command options" matches the actual scope better now.

Once 1–4 are sorted and the formatting is reverted, happy to re-review. npm test (with linting) should catch 2 and 4 locally.

watersRand · 2026-06-27T14:50:24Z

A few things are still open before this can go in:

RESP default regression (commander.ts). const RESP = config?.RESP ?? 2 should stay ?? DEFAULT_RESP (which is 3). The library default is RESP3, and this binds RESP2 transformReply handlers at class-generation time for any client created without an explicit RESP — even though the connection still negotiates RESP3. Bugbot re-flagged this on the latest commit. Please revert to DEFAULT_RESP (and keep the import).

Sentinel getter recurses / duplicate member (sentinel/index.ts). The added
get _commandOptions() { return this.commandOptions; }

recurses — the existing commandOptions getter already returns this._commandOptions, so commandOptions → _commandOptions → commandOptions → … overflows the stack. It also collides with the existing private _commandOptions? field (duplicate identifier), in both RedisSentinelClient and RedisSentinel. Sentinel already stores options in #commandOptions with a public commandOptions getter, so it needs a different shape than the standalone client. Suggest having the sentinel command funcs read this._self.commandOptions (the existing public getter) rather than inventing _commandOptions, or wiring the namespace _self the way commander.ts does — but not a self-referential getter.

Cluster withCommandOptions not updated (cluster/index.ts). Still proxy._commandOptions = options; (plain replace), unlike client/pool which now do Object.assign(Object.create(this._commandOptions ?? null), options). Since the cluster command paths now read this._commandOptions, chaining withCommandOptions drops parent options like timeout. Please make it consistent.

Duplicate import (cluster/index.spec.ts). import { RESP_TYPES } from '../RESP/decoder' appears twice. Won't compile.

Unrelated formatting churn. Most of the diff is reformatting (noop, if( → if (, reindenting the .on() chain and the _ejectSocket block, type-map indentation) in files this fix doesn't functionally touch. It buries the real change and invites conflicts — please strip it and keep only the functional edits. If formatting is genuinely off, a separate PR is the place for it.

Also minor: the title still says "ensure module commands respect proxy typeMapping" — "proxies respect command options" matches the actual scope better now.

Once 1–4 are sorted and the formatting is reverted, happy to re-review. npm test (with linting) should catch 2 and 4 locally.

Thanks for the review! I acknowledge all the points and have updated the PR accordingly. The regressions, sentinel stack overflow, cluster inheritance logic, and duplicate imports are now fixed, and I've completely stripped out the unrelated formatting noise.

- Revert sentinel utils/types to master: sentinel command functions already resolve per-proxy options via the public `commandOptions` getter, and the added intersection types collided with the classes' private `_commandOptions`, breaking the build after merging master. - Replace explicit `any` casts in the client/pool/cluster specs with typed `WithCommandOptions` casts (no-explicit-any). - Adapt the spread-based regression tests to the prototype-chain design: dispatch paths read individual properties, so inherited overrides stay reachable without flattening at proxy creation. - Use `abortSignal` (the real CommandOptions key) instead of `signal` in the cluster proxy test. - Forward `_commandOptions` on namespaces without coercing `undefined` to `null`, and tidy formatting of the new code blocks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

nkaradzhov · 2026-07-10T17:20:03Z

Thank you for the persistence on this one, and for addressing all the points from the earlier reviews — the latest revision is much cleaner.

Unfortunately, this has been overtaken by events: the underlying bug class was fixed on master by #3295 (merged in May, released in 6.0.0). That change fixed the sendCommand merge paths on client/cluster/pool/sentinel and switched the namespace cache to per-receiver binding, so module and function commands dispatched through withCommandOptions(...) / withTypeMapping(...) proxies now resolve the proxy's options. Master also carries regression tests for these paths (proxy → raw sendCommand, typed commands, namespace _self binding, chained overrides, duplicate()).

After merging master into this branch, what remains here is effectively a redesign of the option-merge strategy from flat spreads to prototype-chain layering. We deliberately chose flat merges in #3295: with a prototype chain, any consumer that spreads an options object silently drops inherited keys, and the extra machinery this PR needs (#flattenCommandOptions for duplicate(), the per-attempt executionClient wrapper in cluster _execute) illustrates the cost of maintaining that invariant. Since there's no remaining user-facing defect this fixes, I'm going to close this rather than take on the added complexity.

Sorry the timing worked out this way — the effort here genuinely helped pin down the scope of the problem, and the review discussion fed directly into what got fixed. Contributions are very welcome anytime.

cursor Bot reviewed May 2, 2026

View reviewed changes

Comment thread packages/client/lib/client/index.ts

Comment thread packages/client/lib/client/index.ts Outdated

cursor Bot reviewed May 2, 2026

View reviewed changes

Comment thread packages/client/lib/client/index.spec.ts

cursor Bot reviewed May 2, 2026

View reviewed changes

Comment thread packages/client/lib/commander.ts Outdated

cursor Bot reviewed May 5, 2026

View reviewed changes

Comment thread packages/client/lib/commander.ts Outdated

nkaradzhov requested changes May 7, 2026

View reviewed changes

cursor Bot reviewed May 31, 2026

View reviewed changes

Comment thread packages/client/lib/sentinel/utils.ts Outdated

Comment thread packages/client/lib/client/pool.ts Outdated

cursor Bot reviewed May 31, 2026

View reviewed changes

Comment thread packages/client/lib/client/pool.ts Outdated

Comment thread packages/client/lib/cluster/index.ts Outdated

cursor Bot reviewed Jun 25, 2026

View reviewed changes

Comment thread packages/client/lib/commander.ts Outdated

nkaradzhov self-requested a review June 25, 2026 11:00

nkaradzhov requested changes Jun 25, 2026

View reviewed changes

watersRand added 4 commits June 27, 2026 12:17

Add proxy inheritance for clients

46e5eff

Add proxy for sendCommand

183c970

Preserve inheritance in duplicated clients

c222421

Nested cluster prototype inheritance

c2de21e

watersRand changed the title ~~fix: ensure module commands respect proxy typeMapping~~ Ensure proxies respect command options Jun 27, 2026

watersRand force-pushed the fix/withTypeMappings branch from fa944a8 to c2de21e Compare June 27, 2026 14:51

watersRand and others added 3 commits July 8, 2026 17:27

update tests

67744f1

Merge remote-tracking branch 'upstream/master' into pr-3261-tmp

2030044

nkaradzhov closed this Jul 10, 2026

		});


		testUtils.testWithClient('Module TypeMapping Fix', async client => {

Uh oh!

Conversation

watersRand commented May 2, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkaradzhov commented May 7, 2026

Uh oh!

nkaradzhov left a comment

Choose a reason for hiding this comment

Uh oh!

nkaradzhov May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nkaradzhov commented Jun 25, 2026

Uh oh!

nkaradzhov left a comment

Choose a reason for hiding this comment

Uh oh!

watersRand commented Jun 27, 2026

Uh oh!

nkaradzhov commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

watersRand commented May 2, 2026 •

edited by cursor Bot

Loading